-
Notifications
You must be signed in to change notification settings - Fork 390
Make "frame" spans go on a separate trace line than others #4451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
src/bin/log/tracing_chrome.rs
Outdated
.id() | ||
.into_u64() | ||
), | ||
_ => if span.metadata().name() == "frame" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not a fan of hard-coding a particular name here, that way lies total spaghetti code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I've made it more general
"frame" spans indicate stack frames in the interpreted program
7b27a2f
to
d7ec893
Compare
@rustbot ready |
You just moved the "frame" name checking to a different file, I don't see how this is better. The "frame" span itself should have appropriate metadata that controls this behavior, rather than special-casing the name after it has already entered the logging system. IOW, the code here is in charge of saying what happens with |
I don't fully understand neither |
Yeah I agree, and the right way to do this would be to use the
The problem with One idea me and Max had during our last meeting is to allow enabling/disabling spans from specific modules at runtime, in the same way that text logs from specific modules be enabled/disabled using the |
tracing_chrome
supports two styles of data that are shown differently in https://ui.perfetto.dev:I believe the "Threaded" style generally allows for more insights as argued here, however it's a bit cumbersome to use because the "frame" spans (i.e. those that indicate stack frames in the interpreted program) are very big with respect to other spans and are therefore in the way. So this PR puts them (and only them) on a separate trace line:
Ideally this kind of filtering should be doable from the trace viewer UI, but I don't think https://ui.perfetto.dev supports anything like that.
Let me know if I should make the changes more general, e.g. by adding a new<- this was doneTraceStyle
likeThreadedWithExceptions(Vec<&'static str>)
instead of a hardcodedif
. I don't think we're going to need this for other spans though.